Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move metric initialisation upfront connection creation to avoid potential npe #11

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

philnate
Copy link

@philnate philnate commented Jan 7, 2016

We have a setup where we regularly create a new Memcached client to verify memcached is still running. Now what happens if you totally cut down your network (no LAN & WLAN). You'll get a NPE within MemcachedConnection as the first connection try fails, and then the failure handling tries again trying to increment a metric, which at this time isn't init'd.

java.lang.NullPointerException
    at net.spy.memcached.MemcachedConnection.queueReconnect(MemcachedConnection.java:714)
    at net.spy.memcached.MemcachedConnection.createConnections(MemcachedConnection.java:241)
    at net.spy.memcached.MemcachedConnection.<init>(MemcachedConnection.java:174)
    at net.spy.memcached.DefaultConnectionFactory.createConnection(DefaultConnectionFactory.java:196)
    at net.spy.memcached.MemcachedClient.<init>(MemcachedClient.java:207)

This alone wouldn't be too bad if during init a Selector was opened already, which then isn't closed any more. So on mac each time the Selector is opened 2 PIPEs and 1 KQUEUE are created, this leads eventually to a create AF_UNIX socket: Too many open files in system error making the whole system unresponsive.

  public MemcachedConnection(final int bufSize, final ConnectionFactory f,
      final List<InetSocketAddress> a, final Collection<ConnectionObserver> obs,
      final FailureMode fm, final OperationFactory opfactory) throws IOException {
    //...
    selector = Selector.open();

I've fixed the NPE by moving the metric init to an earlier place, but maybe some try catch around the whole init method would be better to close the all potentially opened handles before bubbling the exception.

@philnate
Copy link
Author

@daschl any plans to merge this into the main branch?

@daschl
Copy link

daschl commented Feb 15, 2017

@philnate yeah I think we can merge something like this, but we should also see how we can address the NPE, right?

@philnate
Copy link
Author

Sounds good. Well my understanding was that the ordering of the commands where wrong. So the proposed solution would be to make sure the metrics are initialized before the connection is tried to be establised. What else should be addressed regarding the NPE, that looked like the main reason to me?

@michaelkwan
Copy link

Is there no plans to fix this?

favila pushed a commit to useshortcut/spymemcached that referenced this pull request Jan 14, 2022
migrate log4j 1.2.x to log4j 2.13.3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants